fix(memory): skip non-dict rows in extract_schema_items/describe - #2586

Walkthrough

The schema indexer now normalizes malformed nested manifest fields, skips invalid entries during description and extraction, and filters model and cube summaries to valid named dictionary children. New tests cover malformed rows, scalar collections, and invalid top-level sections.

Changes

Schema indexing safeguards

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

• Canner/WrenAI#2515: Related defensive handling for malformed column entries.
• Canner/WrenAI#2533: Similar defensive filtering in schema indexing.
• Canner/WrenAI#2540: Related handling of malformed schema entries.

Poem

I guard the schema gate,
Bad rows hop away.
Named fields fill the nest,
Tests confirm the rest!

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

_{Comment @coderabbitai help to get the list of available commands.}

Thanks @coderabbitai — good catches, all three addressed in the latest push:

• Added a small _as_list() helper so collection fields that hold a truthy scalar (e.g. a number or mapping) no longer slip past value or [] and raise TypeError on iteration. Applied it consistently across models/columns/relationships/views/cubes in both describe_schema and extract_schema_items.
• Column extraction now skips entries with a missing/empty name, matching the existing _model_record validation.
• Strengthened the test: exact record count + per-item type/name assertions, and a new regression case for truthy-scalar collections.

All 3 tests pass; ruff format + check clean.

@Bartok9 Thanks for the thorough follow-up. The defensive list normalization, column-name validation, and focused regression coverage address all three findings.

🐇✨

If you found this review helpful, would you consider giving us a shout-out on X?

Thank you for using CodeRabbit!

#!/bin/bash
set -euo pipefail

git diff --check
git diff -- core/wren/src/wren/memory/schema_indexer.py core/wren/tests/unit/test_schema_indexer_extract_nonduct.py

ast-grep outline core/wren/src/wren/memory/schema_indexer.py --items all \
  --match '_as_list|describe_schema|extract_schema_items|_model_record'
ast-grep outline core/wren/tests/unit/test_schema_indexer_extract_nonduct.py --items all

rg -n -A45 -B8 \
  'def _as_list|def describe_schema|def extract_schema_items|def _model_record|def test_extract_skips_non_dict_models_columns_rels_views_without_raise|def test_describe_skips_non_dict_rows' \
  core/wren/src/wren/memory/schema_indexer.py \
  core/wren/tests/unit/test_schema_indexer_extract_nonduct.py

@Bartok9 Confirmed—each of the three fixes is present in the latest patch:

• _as_list() prevents truthy non-list collection values from being iterated.
• Invalid or unnamed columns are consistently excluded during extraction and model summary construction.
• The tests now assert the exact surviving records and cover malformed truthy scalar collections.

git diff --check is also clean. Thanks for the thorough follow-up! 🐇

If you found this review helpful, would you consider giving us a shout-out on X?

Thank you for using CodeRabbit!

I traced this one properly, and unlike some of the others in this batch the guards here are reachable — two ways:

1. wren memory index --mdl <path> → _load_manifest() reads whatever JSON the user points at (memory/cli.py:57) → index_schema(manifest) → extract_schema_items. That's raw user input with nothing in between.
2. Even without --mdl, target/mdl.json comes from save_target(build_json(project)), and build_manifest (context.py:786-807) drops load_models(project_path) into the manifest verbatim — _convert_keys only renames keys, it doesn't coerce values. So columns: 42 in a hand-edited metadata.yml survives all the way into extract_schema_items.

So a truthy non-list nested collection is a real input, and the tests are the right shape too — they call extract_schema_items / describe_schema directly, cover columns: 42 / measures: 3 / dimensions: {"x": 1} / timeDimensions: "nope", and pin that the top-level _iter_section raise still happens. No complaints there.

One thing to settle before merge: this contradicts #2605 on policy. Your own _as_list docstring names the asymmetry it introduces —

Top-level MDL sections still go through _iter_section (raises on wrong types) so a mistyped root key cannot silently empty the index.

— but #2605 adds _relationship_models to the same file and picks the opposite policy for a nested field: a non-list relationship.models raises ValueError. So after both land, schema_indexer would silently empty a non-list columns while hard-failing a non-list models, with no rule distinguishing them.

I'd rather have one rule. The argument for raising on both is that memory/cli.py:224 already wires the error path — it catches ValueError and exits with Malformed manifest: {e} — so a mistyped columns: would tell the user what's wrong in their project instead of quietly indexing a model with zero columns and leaving them to wonder why recall is worse. Silent-empty here also loses the same information _iter_section was written to preserve.

Could you and #2605 converge on one of those? Whichever you pick, _as_list and _relationship_models should follow it. Also: both PRs touch schema_indexer.py, so whichever lands second will need a rebase — they don't overlap textually today, but the second one should re-run the first's tests rather than assume.

Thanks @goldmedal — appreciate you tracing the reachability; agreed both paths (--mdl and the build_manifest verbatim copy) make a non-list nested collection a real input.

On the policy split with #2605: I'll converge on raise, not silent-empty. Your argument is the right one — memory/cli.py:224 already catches ValueError → Malformed manifest: {e}, so raising on a mistyped columns: tells the user exactly what's wrong in their project instead of quietly indexing zero columns and leaving them to wonder why recall degraded. That also matches what _iter_section was written to preserve, and it keeps one rule across _as_list and #2605's _relationship_models.

I'll update this PR so _as_list raises ValueError on a truthy non-list nested value (keeping the None/empty passthrough), update the docstring to drop the asymmetry note, and adjust the tests to assert the raise. Since both PRs touch schema_indexer.py, whichever lands second I'll rebase and re-run the other's tests rather than assume no interaction. Will push shortly.

Pushed dc3ed2c with the converged policy:

• _as_list now raises ValueError on a truthy non-list nested collection (e.g. columns: 42, measures: 3, dimensions: {...}, timeDimensions: "nope"); None/missing/empty still pass through as [].
• Threaded a field name so the message is manifest columns must be a list, got int, which surfaces cleanly through memory/cli.py's Malformed manifest: {e}.
• Dropped the asymmetry note from the docstring — it now states the single rule shared with _iter_section and fix(memory): reject non-list relationship models in schema_indexer #2605's _relationship_models.
• Tests updated: the old silent-empty test is replaced by one asserting the raise per field, plus a passthrough test for None/empty.

Since both PRs touch schema_indexer.py, whichever lands second I'll rebase and re-run the other's tests. This one's now the raise-everywhere half of the agreement.

Re-reviewed at e5e8a1b. Thanks for taking the raise route — that's the right call, and the tests now assert the policy per field name rather than just "doesn't crash".

But the "one rule across the module" claim isn't true yet — falsy non-lists still diverge. _as_list short-circuits on any falsy value:

if value is None or not value:
    return []

Compare the two helpers it says it matches, both of which special-case None only:

So columns: {} in a hand-edited metadata.yml — at least as likely a typo as columns: 42, since it's what you get from a half-deleted mapping — silently indexes a model with zero columns. That is verbatim the outcome your own docstring gives as the reason the helper raises:

a truthy non-list ... is a structural error in the manifest, so we raise ValueError rather than silently indexing a model with zero columns

One line fixes it:

if value is None:
    return []

[] still returns [] via the isinstance check below, so the empty case is unaffected. It also drops a redundancy — in value is None or not value, the first clause is already subsumed by the second.

The tests are why it slipped. test_extract_allows_none_or_empty_nested_collections covers None and [], and the raising test covers 42 / 3 / {"x": 1} / "nope" — all truthy. Nothing exercises {} / 0 / "", so the divergence is invisible. Worth adding them to test_extract_raises_on_truthy_scalar_nested_collections (and renaming it, since the rule would no longer be about truthiness).

Smaller things:

• field: str = "field" — the default is unreachable, all ten callsites pass an explicit name, and if it ever were hit the message reads "manifest field must be a list". Make it a required positional.
• Message format diverges from the helper it's aligning with: _iter_section emits manifest['models'] must be a list, got dict, _as_list emits manifest columns must be a list, got dict. Same error class, two shapes — worth unifying in a PR whose thesis is one rule.
• assert len(items) == 4 is brittle for a count that changes whenever a record type is added; the by_type assertions below it already carry the intent.

One behavioural note for the description/CHANGELOG: the new raise widens where ValueError can surface. memory/cli.py:224 catches it (Malformed manifest: + exit 1) and watch.py:126 catches broadly, so both are fine. But Memory.describe_schema / Memory.index_schema (memory/__init__.py:42-51) delegate without catching, so SDK callers now get a ValueError where a nested non-list previously passed through silently. Intended, but it's a public-surface change worth naming.

Fix the not value line and this is ready. Since #2605 lands in the same file, whichever goes second should re-run the other's tests rather than assume the merge is clean.

Pushed 3afd0a8 — you're right, the not value short-circuit made the "one rule" claim false for falsy non-lists.

• _as_list now returns [] only for None; [] still passes via the isinstance check. Every other non-list raises — so columns: {} / 0 / "" now raise exactly like 42, matching _iter_section and fix(memory): reject non-list relationship models in schema_indexer #2605's _relationship_models. Dropped the redundant is None or not value.
• field is now a required positional (all callsites already pass a name; the unreachable default is gone).
• Message unified to _iter_section's shape: manifest['columns'] must be a list, got dict.
• Tests: renamed to test_extract_raises_on_non_list_nested_collections and added the falsy cases ({}, 0, "") that were invisible before; dropped the brittle len(items) == 4 (the by_type asserts carry the intent).
• CHANGELOG/description note: the widened raise reaches SDK callers via Memory.describe_schema/index_schema (they delegate without catching) — intended public-surface change, now named.

Rebased onto your ruff-format tip (e5e8a1b). Couldn't run the suite locally (wren_core native binding isn't built in my env) but the helper logic is verified standalone and it py-compiles clean. Since #2605 touches the same file, I'll re-run its tests on whichever lands second.

Re-reviewed at 9d6ab81. Everything from the last round is addressed, and correctly:

• _as_list now short-circuits on None only, so {} / 0 / "" raise like every other non-list.
• field is a required positional — no unreachable "field" default.
• Message is manifest['columns'] must be a list, got dict, matching _iter_section's format exactly.
• Tests cover the falsy non-lists, and the matchers are anchored per field name rather than a bare "must be a list".
• The brittle assert len(items) == 4 is gone; the by_type assertions carry it.

CI is green across all ten checks.

One thing left, and it's about the docstring's claim rather than the code. It says:

This matches :func:_iter_section for top-level sections and the same-file _relationship_models policy (see #2605): one rule across the module

_relationship_models doesn't exist on this branch — it arrives with #2605. Standing alone, this PR leaves _describe_relationship (line 190) and _relationship_record (line 412) on the old form:

models = rel.get("models") or []

So if this merges first, the module ends up with two rules, and the surviving one is the worse of the two: columns: {} raises a clean Malformed manifest, while models: "orders" still silently renders orders → o / r, because models[0] / models[1] index into the string. That's the regression #2605's own test names ("str models used to index first two characters as endpoints").

Either fix works:

1. Merge fix(memory): reject non-list relationship models in schema_indexer #2605 first and rebase this on top — then the docstring is accurate as written, and the module genuinely has one rule.
2. Or reword to say the policy is intended to converge with fix(memory): reject non-list relationship models in schema_indexer #2605, so the comment doesn't assert something the branch doesn't do yet.

I'd prefer (1) — it also means the field-named error message and _relationship_models' relationship {name!r}: 'models' must be a list wording get reconciled in one place while you're both in the file, rather than landing two formats for the same error class.

Otherwise this is ready.

#2605 is merged (73c1255), so following up here as promised.

Good news on the docstring. _relationship_models is now in main at schema_indexer.py:165, so this PR's _as_list docstring no longer forward-references something that doesn't exist — the "one rule across the module" claim is true once you're on current main. GitHub still reports this branch MERGEABLE, so no conflict to resolve; just rebase and re-run so #2605's tests run alongside yours.

Now the correction I owe you on the error message. Two rounds ago I asked you to match _iter_section's format, and you did:

manifest['columns'] must be a list, got dict

Having reviewed #2605 next to this, I got that backwards and I'd like you to reverse it. What's now in main is:

relationship 'orders_customers': 'models' must be a list, got str

That names the offending entity, which is what someone editing their project actually needs. manifest['columns'] must be a list tells them a column list is wrong somewhere across every model in the project — for a manifest with fifty models that's a search, not a diagnosis. _iter_section has the excuse that a top-level section has no entity to name; a nested field does.

So the rule I'd like across the module:

Concretely, _as_list grows the entity the way _relationship_models(rel, name) ended up doing — every callsite already has it: _describe_model / _model_record have model["name"], and _describe_cube / _cube_record have cube.get("name", ""). Something like _as_list(value, kind, name, field), giving model 'orders': 'columns' must be a list, got dict and cube 'revenue': 'measures' must be a list, got int.

One edge case to decide while you're at it. Models are required to have a name, but cubes aren't — both describe_schema:86-88 and extract_schema_items:288-292 only check isinstance(cube, dict), and cube_name is cube.get("name", ""). So a nameless cube with measures: 3 would render cube '': 'measures' must be a list, which is barely better than not naming it. Either fall back to the manifest['measures'] form when the name is empty, or say cube (unnamed) — your call, just make it deliberate rather than an accidental ''.

The test matchers are currently anchored to manifest\['columns'\], so they'll need to move with the format. Please keep them as tight as they are now — per-field and per-type, the way #2605 ended up.

Sorry for the round-trip on this one; the format only became obvious with both helpers side by side.

Pushed 106b4d8 — rebased on current main (with #2605's _relationship_models landed), and took the entity-named format you asked for plus the edge case.

• Nested errors name the entity now, per your table: model 'orders': 'columns' must be a list, got dict / cube 'revenue': 'measures' must be a list, got int. Top-level sections keep the manifest['models'] form via _iter_section. So the surviving rule across the module matches fix(memory): reject non-list relationship models in schema_indexer #2605 exactly.
• Unnamed-cube edge case — made deliberate. Cubes aren't required to have a name, so when the name is empty _as_list falls back to the top-level manifest['measures'] form rather than emitting a bare cube '':. New test test_extract_unnamed_cube_falls_back_to_manifest_form pins it.
• Docstring reworded so it no longer forward-references — it now states the entity-naming + unnamed-cube policy as-is on main.
• Tests tightened per CodeRabbit + your ask: added the dict-column-missing-name skip case (test_extract_skips_columns_missing_name), and test_describe_skips_non_dict_rows now uses unique sentinels asserted absent from the rendered text. Matchers stay per-field / per-type.

ruff format + check clean, py-compiles clean. Couldn't run the suite locally (wren_core native binding isn't built in my env) but the _as_list branches are verified standalone. No merge conflict — GitHub still reports MERGEABLE.

Thanks for the side-by-side correction — the entity-named form is clearly the right call for a nested field.

Re-reviewed at 106b4d8. Rebased onto the merged #2605 (the base blob is the post-73c1255 file), CI green, and the format change is applied consistently — all twelve _as_list callsites pass kind / name / field, and the two that can't guarantee a name (_describe_cube, _cube_record) route through the fallback. _describe_model / _model_record are safe to pass name unconditionally since both callers already require a truthy model["name"].

Two things left, and the first one is a suggestion of mine that I now think was the wrong half of the choice I gave you.

1. The unnamed-cube fallback invents a manifest key that doesn't exist. I offered either manifest['measures'] or cube (unnamed) and you took the first, so this is on me — but seeing it concretely:

manifest['measures'] must be a list, got int

_iter_section's manifest['models'] works because models really is a top-level key someone can go look at. measures isn't — it only ever exists nested inside a cube. So for the one case where we've lost the entity name, the message now sends the reader hunting for a top-level measures: in their mdl.json that was never there. That's the opposite of what this PR is for.

cube (unnamed): 'measures' must be a list, got int keeps it in the same family as the named form, stays truthful about where the field lives, and still signals that we couldn't identify which cube. The docstring paragraph about the fallback and test_extract_unnamed_cube_falls_back_to_manifest_form would move with it.

2. The matchers lost the type half. I asked for these to stay as tight as #2605's, and they're per-field but no longer per-type:

with pytest.raises(ValueError, match=r"model 'm': 'columns' must be a list"):

#2605 pins ... must be a list, got dict / got str / got int, so a regression that reported the wrong type name fails there and would pass here. Since the seven cases already enumerate 42 / {} / 0 / "" / 3 / {"x": 1} / "nope" separately, appending the expected got <type> to each matcher costs nothing and makes the two files consistent.

Everything else is settled as far as I'm concerned — the policy, the sweep, the falsy handling, the entity naming, the passthrough cases, and the extra test_extract_skips_columns_missing_name are all good. Push those two and I'll approve and merge.

Pushed ac1898d — both settled.

1. Unnamed cube no longer invents a top-level key. You're right — manifest['measures'] sends the reader hunting for a root measures: that never exists. The empty-name fallback is now cube (unnamed): 'measures' must be a list, got int — same family as the named form, truthful about where the field lives, and still signals we couldn't identify the cube. Docstring and test_extract_unnamed_cube_falls_back_to_unnamed_form moved with it.

2. Matchers are per-type again. Every case in test_extract_raises_on_non_list_nested_collections now pins ... must be a list, got <type> (got int / got dict / got str), matching #2605's shape so a wrong type-name regression fails here too.

ruff format + check clean, py-compiles clean. Couldn't run the suite locally (wren_core native binding isn't built in my env), but the _as_list branches are verified standalone. No conflict — still MERGEABLE on current main. Thanks again for the side-by-side; the (unnamed) form is clearly right.